Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPUBackend: Make guest RIP reconstruction offsets signed #4271

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

bylaws
Copy link
Collaborator

@bylaws bylaws commented Jan 12, 2025

With multiblock enabled, host code generated from guest code with a lower address may be placed after host code generated from guest code with a higher address in a multiblock. As each guest RIP reconstruction entry is always relative to the one before it the offset needs to be signed to allow this.

@bylaws bylaws force-pushed the soff branch 2 times, most recently from 06ea691 to 1a7a93a Compare January 14, 2025 20:29
@@ -875,6 +876,13 @@ CPUBackend::CompiledCode Arm64JITCore::CompileCode(uint64_t Entry, uint64_t Size
for (size_t i = 0; i < DebugData->GuestOpcodes.size(); i++) {
const auto& GuestOpcode = DebugData->GuestOpcodes[i];
auto& RIPEntry = JITRIPEntries[i];
#if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED
Copy link
Member

@neobrain neobrain Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checks here are trivial (particularly compared to the two memory stores right after), so there's no need to restrict them to assertion-builds.

(Actually I'm just now realizing LOGMAN_THROW_A(A)_FMT are already defined as no-ops for non-ASSERTION builds, so adding this extra check here is doubly unnecessary :( )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a code pattern used throughout FEX, unsure why

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason either, but a bad pattern being widespread doesn't make a compelling reason to stick to it either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll clean it up in general

Copy link
Member

@neobrain neobrain Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, there's a certain irony in using __builtin_assume annotations for speculative performance gains only if the macro to enable expensive assertions is active.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern here of wrapping LOGMAN_THROW_{A,AA}_FMT usage in additional #if defined(ASSERTIONS_ENABLED) && ASSERTIONS_ENABLED wrappers only matters in non-trivial places.

Like in ThreadManager.cpp this happens because it ensures correctness by locking a mutex and walking an array. So it does both, but we don't want that mutex locking in release builds.

FEXCore/Source/Interface/Core/JIT/JIT.cpp Show resolved Hide resolved
Copy link
Member

@neobrain neobrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pick up the discussion about __builtin_assume somewhere else. The unnecessary ASSERTIONS_ENABLED noise should probably go, but other than that this looks good!

With multiblock enabled, host code generated from guest code with a
lower address may be placed after host code generated from guest code
with a higher address in a multiblock. As each guest RIP reconstruction
entry is always relative to the one before it the offset needs to be
signed to allow this.
@Sonicadvance1 Sonicadvance1 merged commit 981eea6 into FEX-Emu:main Jan 17, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants